Split epics make record #319
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #319 +/- ##
==========================================
+ Coverage 90.82% 90.90% +0.07%
==========================================
Files 70 70
Lines 2551 2572 +21
==========================================
+ Hits 2317 2338 +21
Misses 234 234 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
Looks good. Minor comments.
📝 WalkthroughWalkthroughSplit the unified Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@GDYendell ready. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/transports/epics/ca/test_softioc.py (1)
547-578:⚠️ Potential issue | 🟡 MinorAssert the out-record builder after switching to _make_out_record.
The test at line 571 still assertsbuilder.longInwhen it should assertbuilder.longOut, as_make_out_recorduses the out-record builder for write attributes. The assertion also needs to include the correct parameters.🔧 Proposed fix
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
🤖 Fix all issues with AI agents
In `@tests/transports/epics/ca/test_initial_value.py`:
- Around line 68-74: The wait loop condition currently uses "and" which exits as
soon as one list is non-empty; change the loop to wait until both spies have
populated lists by using a condition that continues while either list is empty
(e.g., while not (record_spy.spy_return_list and
record_spy_out.spy_return_list)) so initial_values built from
record_spy.spy_return_list + record_spy_out.spy_return_list is complete; update
the while loop surrounding the asyncio.sleep(0) accordingly (refer to the
variables record_spy, record_spy_out and the initial_values comprehension).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/transports/epics/ca/test_softioc.py (1)
546-583:⚠️ Potential issue | 🟡 MinorAssert
longOutfor output records in datatype update test.After switching to
_make_out_record, the test still assertsbuilder.longIn, which doesn’t validate the output record builder path. Consider assertinglongOutwithout_record=Truemetadata instead. Line 573.✅ Suggested test fix
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 241-301: datatype_updater currently passes all items from
record_metadata_from_datatype(..., out_record=True) into record.set_field, but
that metadata can include a callable under the "validate" key (for long Enums)
which RecordWrapper.set_field cannot accept; update the datatype_updater closure
to filter out the "validate" key (or any non-scalar/callable values) before
calling record.set_field so only EPICS DB field values are set; ensure this
change is applied where datatype_updater is added via
attribute.add_update_datatype_callback so long Enum handling and other datatypes
remain correct.
| def _make_out_record( | ||
| pv: str, | ||
| attribute: AttrR | AttrW | AttrRW, | ||
| on_update: Callable, | ||
| ) -> RecordWrapper: | ||
| datatype_record_metadata = record_metadata_from_datatype( | ||
| attribute.datatype, out_record | ||
| attribute.datatype, out_record=True | ||
| ) | ||
| attribute_record_metadata = record_metadata_from_attribute(attribute) | ||
|
|
||
| update = ( | ||
| {"on_update": on_update, "always_update": True, "blocking": True} | ||
| if on_update | ||
| else {} | ||
| ) | ||
| update = {"on_update": on_update, "always_update": True, "blocking": True} | ||
|
|
||
| match attribute.datatype: | ||
| case Bool(): | ||
| record = builder.boolOut( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| case Int(): | ||
| record = builder.longOut( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| case Float(): | ||
| record = builder.aOut( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| case String(): | ||
| record = builder.longStringOut( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| case Enum(): | ||
| if len(attribute.datatype.members) > MBB_MAX_CHOICES: | ||
| record = builder.longStringOut( | ||
| pv, | ||
| **update, | ||
| **datatype_record_metadata, | ||
| **attribute_record_metadata, | ||
| ) | ||
|
|
||
| record = builder_callable( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| else: | ||
| record = builder.mbbOut( | ||
| pv, | ||
| **update, | ||
| **datatype_record_metadata, | ||
| **attribute_record_metadata, | ||
| ) | ||
| case Waveform(): | ||
| record = builder.WaveformOut( | ||
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | ||
| ) | ||
| case _: | ||
| raise FastCSError( | ||
| f"EPICS unsupported datatype on {attribute}: {attribute.datatype}" | ||
| ) | ||
|
|
||
| def datatype_updater(datatype: DataType): | ||
| for name, value in record_metadata_from_datatype(datatype, out_record).items(): | ||
| for name, value in record_metadata_from_datatype( | ||
| datatype, out_record=True | ||
| ).items(): | ||
| record.set_field(name, value) | ||
|
|
||
| attribute.add_update_datatype_callback(datatype_updater) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the record_metadata_from_datatype function
fd -t f "\.py$" | xargs rg -l "def record_metadata_from_datatype"Repository: DiamondLightSource/FastCS
Length of output: 107
🏁 Script executed:
# Also search for RecordWrapper to understand the API
fd -t f "\.py$" | xargs rg -l "class RecordWrapper"Repository: DiamondLightSource/FastCS
Length of output: 53
🏁 Script executed:
# Check the specific file and surrounding context
cat -n src/fastcs/transports/epics/ca/ioc.py | sed -n '240,305p'Repository: DiamondLightSource/FastCS
Length of output: 2749
🏁 Script executed:
# Let's look at the record_metadata_from_datatype function
cat -n src/fastcs/transports/epics/ca/util.py | head -200Repository: DiamondLightSource/FastCS
Length of output: 6288
🏁 Script executed:
# Search for the record_metadata_from_datatype function specifically
rg -A 50 "def record_metadata_from_datatype" src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 1814
🏁 Script executed:
# Search for "validate" in the util.py file
rg "validate" src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 428
🏁 Script executed:
# Search for RecordWrapper usage to understand the API better
rg "RecordWrapper" src/fastcs/transports/epics/ca/ioc.py -B 2 -A 2Repository: DiamondLightSource/FastCS
Length of output: 772
🏁 Script executed:
# Search for set_field calls in the codebase
rg "set_field" src/fastcs/ -B 2 -A 2Repository: DiamondLightSource/FastCS
Length of output: 838
🏁 Script executed:
# Check if there are any test files that test this scenario
fd -t f "test.*\.py$" -o ".*_test\.py$" | xargs rg -l "record_metadata_from_datatype\|datatype_updater" 2>/dev/null || echo "No test files found with these functions"Repository: DiamondLightSource/FastCS
Length of output: 270
🏁 Script executed:
# Search for imports and usage of pythonSoftIoc to understand RecordWrapper
rg "pythonSoftIoc\|RecordWrapper" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 51
🌐 Web query:
pythonSoftIoc RecordWrapper set_field API documentation
💡 Result:
RecordWrapper.set_field() (the object returned by softioc.builder.* record-creation calls, i.e., ProcessDeviceSupportIn / ProcessDeviceSupportOut) is documented in the pythonSoftIOC API reference under “Working with IN records” and “Working with OUT records”. It:
set_field(field, value): sets an EPICS record field by name.- Always converts
valueto a Python string, which EPICS interprets as DBF_STRING. - Values are limited to 39 bytes.
- Can only be used after
iocInit(); before init, set fields via direct attributes (e.g.,my_record.EGU = "mm"), which won’t work after init. [1]
Source: pythonSoftIOC API docs page (“API → softioc.device → set_field”). [1]
🏁 Script executed:
# Check if _make_in_record has similar code pattern with datatype_updater
rg -B 5 -A 20 "def _make_in_record" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 927
🏁 Script executed:
# Look for all datatype_updater closures to see if there are multiple instances
rg -B 3 -A 10 "def datatype_updater" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 903
Filter out "validate" before passing to record.set_field in the datatype_updater closure.
For long Enums (> 16 members), record_metadata_from_datatype(..., out_record=True) includes a "validate" callable. Since RecordWrapper.set_field only handles EPICS DB field values (converted to strings), passing a callable will fail at runtime when datatype_updater is invoked. Filter it out or apply via alternative API.
Suggested fix
def datatype_updater(datatype: DataType):
- for name, value in record_metadata_from_datatype(
- datatype, out_record=True
- ).items():
- record.set_field(name, value)
+ metadata = record_metadata_from_datatype(datatype, out_record=True)
+ validate = metadata.pop("validate", None)
+ for name, value in metadata.items():
+ record.set_field(name, value)
+ if validate is not None and hasattr(record, "validate"):
+ record.validate = validate📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _make_out_record( | |
| pv: str, | |
| attribute: AttrR | AttrW | AttrRW, | |
| on_update: Callable, | |
| ) -> RecordWrapper: | |
| datatype_record_metadata = record_metadata_from_datatype( | |
| attribute.datatype, out_record | |
| attribute.datatype, out_record=True | |
| ) | |
| attribute_record_metadata = record_metadata_from_attribute(attribute) | |
| update = ( | |
| {"on_update": on_update, "always_update": True, "blocking": True} | |
| if on_update | |
| else {} | |
| ) | |
| update = {"on_update": on_update, "always_update": True, "blocking": True} | |
| match attribute.datatype: | |
| case Bool(): | |
| record = builder.boolOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Int(): | |
| record = builder.longOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Float(): | |
| record = builder.aOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case String(): | |
| record = builder.longStringOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Enum(): | |
| if len(attribute.datatype.members) > MBB_MAX_CHOICES: | |
| record = builder.longStringOut( | |
| pv, | |
| **update, | |
| **datatype_record_metadata, | |
| **attribute_record_metadata, | |
| ) | |
| record = builder_callable( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| else: | |
| record = builder.mbbOut( | |
| pv, | |
| **update, | |
| **datatype_record_metadata, | |
| **attribute_record_metadata, | |
| ) | |
| case Waveform(): | |
| record = builder.WaveformOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case _: | |
| raise FastCSError( | |
| f"EPICS unsupported datatype on {attribute}: {attribute.datatype}" | |
| ) | |
| def datatype_updater(datatype: DataType): | |
| for name, value in record_metadata_from_datatype(datatype, out_record).items(): | |
| for name, value in record_metadata_from_datatype( | |
| datatype, out_record=True | |
| ).items(): | |
| record.set_field(name, value) | |
| attribute.add_update_datatype_callback(datatype_updater) | |
| def _make_out_record( | |
| pv: str, | |
| attribute: AttrR | AttrW | AttrRW, | |
| on_update: Callable, | |
| ) -> RecordWrapper: | |
| datatype_record_metadata = record_metadata_from_datatype( | |
| attribute.datatype, out_record=True | |
| ) | |
| attribute_record_metadata = record_metadata_from_attribute(attribute) | |
| update = {"on_update": on_update, "always_update": True, "blocking": True} | |
| match attribute.datatype: | |
| case Bool(): | |
| record = builder.boolOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Int(): | |
| record = builder.longOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Float(): | |
| record = builder.aOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case String(): | |
| record = builder.longStringOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case Enum(): | |
| if len(attribute.datatype.members) > MBB_MAX_CHOICES: | |
| record = builder.longStringOut( | |
| pv, | |
| **update, | |
| **datatype_record_metadata, | |
| **attribute_record_metadata, | |
| ) | |
| else: | |
| record = builder.mbbOut( | |
| pv, | |
| **update, | |
| **datatype_record_metadata, | |
| **attribute_record_metadata, | |
| ) | |
| case Waveform(): | |
| record = builder.WaveformOut( | |
| pv, **update, **datatype_record_metadata, **attribute_record_metadata | |
| ) | |
| case _: | |
| raise FastCSError( | |
| f"EPICS unsupported datatype on {attribute}: {attribute.datatype}" | |
| ) | |
| def datatype_updater(datatype: DataType): | |
| metadata = record_metadata_from_datatype(datatype, out_record=True) | |
| validate = metadata.pop("validate", None) | |
| for name, value in metadata.items(): | |
| record.set_field(name, value) | |
| if validate is not None and hasattr(record, "validate"): | |
| record.validate = validate | |
| attribute.add_update_datatype_callback(datatype_updater) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 291-293: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/fastcs/transports/epics/ca/ioc.py` around lines 241 - 301,
datatype_updater currently passes all items from
record_metadata_from_datatype(..., out_record=True) into record.set_field, but
that metadata can include a callable under the "validate" key (for long Enums)
which RecordWrapper.set_field cannot accept; update the datatype_updater closure
to filter out the "validate" key (or any non-scalar/callable values) before
calling record.set_field so only EPICS DB field values are set; ensure this
change is applied where datatype_updater is added via
attribute.add_update_datatype_callback so long Enum handling and other datatypes
remain correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 295-299: datatype_updater is passing the full dict from
record_metadata_from_datatype(..., out_record=True) into record.set_field, which
can include builder-only keys like validate (used by builder.longStringOut) that
are not settable; update datatype_updater to filter out non-field keys (at
minimum remove "validate") from the metadata dict before iterating/calling
record.set_field (use the output of record_metadata_from_datatype but drop keys
such as "validate" or any known builder-only params) so only actual record field
names are passed to record.set_field.
🧹 Nitpick comments (1)
tests/transports/epics/ca/test_softioc.py (1)
547-572: Assert the out-record builder call to fully cover the new out path.After
_make_out_record, the test reassertslongInwith non‑out metadata, so it doesn’t validate the output builder call orout_record=Truemetadata. Consider assertinglongOutwith the update kwargs to exercise the out‑record path.Suggested test adjustment
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
| def datatype_updater(datatype: DataType): | ||
| for name, value in record_metadata_from_datatype(datatype, out_record).items(): | ||
| for name, value in record_metadata_from_datatype( | ||
| datatype, out_record=True | ||
| ).items(): | ||
| record.set_field(name, value) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | head -20Repository: DiamondLightSource/FastCS
Length of output: 624
🏁 Script executed:
find . -type f -name "*.py" | xargs rg -l "record_metadata_from_datatype"Repository: DiamondLightSource/FastCS
Length of output: 237
🏁 Script executed:
find . -type f -name "*.py" | xargs rg -l "class RecordWrapper|def set_field"Repository: DiamondLightSource/FastCS
Length of output: 53
🏁 Script executed:
rg -A 20 "def record_metadata_from_datatype"Repository: DiamondLightSource/FastCS
Length of output: 1582
🏁 Script executed:
rg -B 3 -A 15 "def set_field"Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
cat -n src/fastcs/transports/epics/ca/util.py | head -100Repository: DiamondLightSource/FastCS
Length of output: 3639
🏁 Script executed:
wc -l src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 111
🏁 Script executed:
rg "validate" src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 428
🏁 Script executed:
rg "DATATYPE_FIELD_TO_RECORD_FIELD" src/fastcs/transports/epics/ca/util.py -B 5 -A 15Repository: DiamondLightSource/FastCS
Length of output: 1445
🏁 Script executed:
sed -n '88,110p' src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 765
🏁 Script executed:
sed -n '1,153p' src/fastcs/transports/epics/ca/util.py | tail -50Repository: DiamondLightSource/FastCS
Length of output: 2234
🏁 Script executed:
cat -n src/fastcs/transports/epics/ca/ioc.py | sed -n '290,310p'Repository: DiamondLightSource/FastCS
Length of output: 830
🏁 Script executed:
rg "set_field" src/fastcs/transports/epics/ca/ioc.py -B 3 -A 3Repository: DiamondLightSource/FastCS
Length of output: 553
🏁 Script executed:
sed -n '260,295p' src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 1383
🏁 Script executed:
head -30 src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 1049
🏁 Script executed:
rg "from softioc|import softioc" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 210
🏁 Script executed:
grep -n "def _create_record\|record =" src/fastcs/transports/epics/ca/ioc.py | head -20Repository: DiamondLightSource/FastCS
Length of output: 878
🏁 Script executed:
rg "validate" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
cat -n tests/transports/epics/ca/test_softioc.py | head -100Repository: DiamondLightSource/FastCS
Length of output: 3733
🏁 Script executed:
rg "set_field" tests/transports/epics/ca/test_softioc.pyRepository: DiamondLightSource/FastCS
Length of output: 409
🏁 Script executed:
rg "datatype_updater\|update_datatype" tests/transports/epics/ca/test_softioc.py -B 3 -A 5Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
rg "Enum" tests/transports/epics/ca/test_softioc.py -B 2 -A 10 | head -80Repository: DiamondLightSource/FastCS
Length of output: 2095
🏁 Script executed:
rg "LongEnum" tests/transports/epics/ca/test_softioc.py -B 3 -A 15Repository: DiamondLightSource/FastCS
Length of output: 1611
🏁 Script executed:
rg "add_update_datatype_callback" tests/transports/epics/ca/test_softioc.py -B 5 -A 10Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
cat -n tests/transports/epics/ca/test_softioc.py | sed -n '120,200p'Repository: DiamondLightSource/FastCS
Length of output: 2911
🏁 Script executed:
rg "too.*many|ALTOGETHER|MANY" tests/transports/epics/ca/test_softioc.py -B 5 -A 15Repository: DiamondLightSource/FastCS
Length of output: 381
🏁 Script executed:
sed -n '198,220p' tests/transports/epics/ca/test_softioc.pyRepository: DiamondLightSource/FastCS
Length of output: 1084
🏁 Script executed:
rg "test_.*datatype\|add_update_datatype" tests/transports/epics/ca/test_softioc.py -iRepository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
cat -n tests/transports/epics/ca/test_softioc.py | tail -150 | head -100Repository: DiamondLightSource/FastCS
Length of output: 4685
🏁 Script executed:
python3 << 'EOF'
# Check what MBB_MAX_CHOICES is
import sys
sys.path.insert(0, '/root/repo/src')
from fastcs.transports.epics.ca.util import MBB_MAX_CHOICES
print(f"MBB_MAX_CHOICES = {MBB_MAX_CHOICES}")
# LongEnum has 18 members, so it exceeds MBB_MAX_CHOICES
long_enum_count = 18 # THIS through RECORDS
print(f"LongEnum member count: {long_enum_count}")
print(f"Is LongEnum > MBB_MAX_CHOICES: {long_enum_count > MBB_MAX_CHOICES}")
EOFRepository: DiamondLightSource/FastCS
Length of output: 187
🏁 Script executed:
rg "MBB_MAX_CHOICES" src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 282
🏁 Script executed:
sed -n '546,620p' tests/transports/epics/ca/test_softioc.pyRepository: DiamondLightSource/FastCS
Length of output: 1959
🏁 Script executed:
rg "def test_update_datatype" tests/transports/epics/ca/test_softioc.py -A 50Repository: DiamondLightSource/FastCS
Length of output: 1835
🏁 Script executed:
cat -n src/fastcs/transports/epics/ca/util.py | sed -n '9,30p'Repository: DiamondLightSource/FastCS
Length of output: 580
🏁 Script executed:
rg "test_update_datatype" tests/transports/epics/ca/test_softioc.py -B 5 -A 80 | grep -i "enum\|validate"Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
# Check if there's a test for long enum datatype updates
rg "LongEnum.*update_datatype\|update_datatype.*LongEnum" tests/transports/epics/ca/test_softioc.pyRepository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
# Search for any runtime errors or issues with validate in set_field
rg "validate.*set_field|set_field.*validate" . -iRepository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
# Check if there are any integration tests or runtime tests that update a long enum
find tests -name "*.py" -exec grep -l "LongEnum" {} \;Repository: DiamondLightSource/FastCS
Length of output: 152
🏁 Script executed:
# Look for any GitHub issues or error messages mentioning set_field and validate
git log --oneline --all | head -30Repository: DiamondLightSource/FastCS
Length of output: 1635
🏁 Script executed:
# Check pythonSoftIoc documentation or code for set_field signature
python3 << 'EOF'
import inspect
try:
from softioc.pythonSoftIoc import RecordWrapper
if hasattr(RecordWrapper, 'set_field'):
sig = inspect.signature(RecordWrapper.set_field)
print(f"set_field signature: {sig}")
if RecordWrapper.set_field.__doc__:
print(f"set_field docstring:\n{RecordWrapper.set_field.__doc__}")
except Exception as e:
print(f"Cannot inspect softioc: {e}")
EOFRepository: DiamondLightSource/FastCS
Length of output: 118
🏁 Script executed:
# Look at the actual code flow - when is datatype_updater called and what happens
rg "add_update_datatype_callback" src/fastcs/ -B 5 -A 5Repository: DiamondLightSource/FastCS
Length of output: 2294
🏁 Script executed:
# Check if set_field is tested with unknown parameters
rg "set_field.*validate|set_field" tests/ -B 2 -A 2Repository: DiamondLightSource/FastCS
Length of output: 1426
🏁 Script executed:
# Look for any error handling or parameter validation in softioc usage
rg "RecordWrapper\|from softioc" src/fastcs/ -A 5Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
# Search for any issues or bug reports mentioning set_field
git log --all --grep="set_field" --onelineRepository: DiamondLightSource/FastCS
Length of output: 117
🏁 Script executed:
# Check if there's documentation about RecordWrapper in the repo
find . -name "*.md" -o -name "*.rst" | xargs grep -l "set_field\|RecordWrapper" 2>/dev/null || echo "No docs found"Repository: DiamondLightSource/FastCS
Length of output: 82
🏁 Script executed:
# Look at what parameters set_field might accept - check if it's a wrapper around a generic dict setter
rg "def set_field|\.set_field\(" tests/transports/epics/ca/test_softioc.py | head -20Repository: DiamondLightSource/FastCS
Length of output: 51
🏁 Script executed:
# One final check - look for any recent changes or fixes related to this
git log --all -p --follow -- src/fastcs/transports/epics/ca/ioc.py | grep -A 10 -B 10 "validate\|set_field\|datatype_updater" | head -60Repository: DiamondLightSource/FastCS
Length of output: 1906
🏁 Script executed:
# Check if pythonSoftIoc is listed in dependencies
cat pyproject.toml | grep -A 5 -B 5 softiocRepository: DiamondLightSource/FastCS
Length of output: 449
🏁 Script executed:
# Look at what version of softioc is required
rg "softioc" pyproject.tomlRepository: DiamondLightSource/FastCS
Length of output: 112
Filter builder-only parameters before passing to set_field on datatype updates.
record_metadata_from_datatype(..., out_record=True) includes a validate callable for long enums (enum members > 16). The datatype_updater function passes this entire dict to record.set_field, but validate is a builder-only parameter that works during record creation (via builder.longStringOut), not as a settable record field. Passing it to set_field during datatype updates will fail.
Filter out validate or any other non-field parameters before calling set_field:
Suggested fix
def datatype_updater(datatype: DataType):
for name, value in record_metadata_from_datatype(
datatype, out_record=True
).items():
+ if name == "validate":
+ continue
record.set_field(name, value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def datatype_updater(datatype: DataType): | |
| for name, value in record_metadata_from_datatype(datatype, out_record).items(): | |
| for name, value in record_metadata_from_datatype( | |
| datatype, out_record=True | |
| ).items(): | |
| record.set_field(name, value) | |
| def datatype_updater(datatype: DataType): | |
| for name, value in record_metadata_from_datatype( | |
| datatype, out_record=True | |
| ).items(): | |
| if name == "validate": | |
| continue | |
| record.set_field(name, value) |
🤖 Prompt for AI Agents
In `@src/fastcs/transports/epics/ca/ioc.py` around lines 295 - 299,
datatype_updater is passing the full dict from
record_metadata_from_datatype(..., out_record=True) into record.set_field, which
can include builder-only keys like validate (used by builder.longStringOut) that
are not settable; update datatype_updater to filter out non-field keys (at
minimum remove "validate") from the metadata dict before iterating/calling
record.set_field (use the output of record_metadata_from_datatype but drop keys
such as "validate" or any known builder-only params) so only actual record field
names are passed to record.set_field.
There was a problem hiding this comment.
@ajgdls I think this is correct, but instead we should just move the creation of the validate function to the record creation Enum case. Does that seem reasonable?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
The nitpick is also correct! The assertion is wrong. 🤖 Fix all issues with AI agents🧹 Nitpick comments (1)
FastCS/tests/transports/epics/ca/test_softioc.py Lines 547 to 570 in 04097f3 Line 566 should assert longOut is called. |
2 participants
This resolves #288.
The first commit removes the original builder_callable_from_attribute function from the util file in ca transport and instead calls the record creation directly in the ioc file.
The second commit then splits the _make_record function into _make_in_record and _make_out_record.
Tests updated at each commit.
Summary by CodeRabbit